feat: bls remote signer #745

shrimalmadhur · 2024-09-04T20:15:55Z

Why are these changes needed?

Checks

I've made sure the lint is passing in this PR.
I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

samlaf

Overall LGTM. Added a bunch of nits

samlaf · 2024-11-12T17:25:56Z

node/config.go

+	BLSRemoteSignerUrl             string
+	BLSPublicKeyHex                string
+	BLSKeyPassword                 string
+	BLSSignerTLSCertFilePath       string
+	UseBLSRemoteSigner             bool


Suggested change

BLSRemoteSignerUrl string

BLSPublicKeyHex string

BLSKeyPassword string

BLSSignerTLSCertFilePath string

UseBLSRemoteSigner bool

BLSRemoteSignerEnabled bool

BLSRemoteSignerUrl string

BLSPublicKeyHex string

BLSKeyPassword string

BLSSignerTLSCertFilePath string

Personally prefer to keep same naming convention for all vars, and to have the enabled var at the top.

Maybe consider creating subconfigs also? The config for the node is crazy long.

samlaf · 2024-11-12T17:27:51Z

node/config.go

@@ -143,14 +152,20 @@ func NewConfig(ctx *cli.Context) (*Config, error) {
 		ethClientConfig = geth.ReadEthClientConfig(ctx)
 	}

+	// check if BLS remote signer configuration is provided
+	useBLSRemoteSigner := ctx.GlobalString(flags.BLSRemoteSignerUrlFlag.Name) != "" && ctx.GlobalString(flags.BLSPublicKeyHexFlag.Name) != ""


Personally prefer to have an explicit flag to turn a feature on/off

samlaf · 2024-11-12T17:31:34Z

node/flags/flags.go

+	BLSPublicKeyHexFlag = cli.StringFlag{
+		Name:     common.PrefixFlag(FlagPrefix, "bls-public-key-hex"),
+		Usage:    "The hex-encoded public key of the BLS signer",
+		Required: false,
+		EnvVar:   common.PrefixEnvVar(EnvVarPrefix, "BLS_PUBLIC_KEY_HEX"),


does bls signer do auth? Or does it just store a bunch of private keys and if I have access to its endpoint then I can just ask it to sign from any private key just by giving it the respective public key?

right now it doesn't support auth - you can ask for any public key hex. but I think we will support it eventually.

node/grpc/server.go

node/node.go

samlaf · 2024-11-12T17:44:42Z

node/node.go

+	if config.PrivateBls != "" {
+		nodeLogger.Info("using local keystore private key for BLS signing")
+		// Generate BLS keys
+		keyPair, err = core.MakeKeyPairFromString(config.PrivateBls)
+		if err != nil {
+			return nil, err
+		}
+
+		config.ID = keyPair.GetPubKeyG1().GetOperatorID()
+	} else {
+		pkBytes, err := hex.DecodeString(config.BLSPublicKeyHex)


personally find this a bit confusing to read.
Is the intention that:

test mode: bls private key

prod mode: bls sk file OR bls remote signer
?

in config.go above you first check if !testmode, then build the bls remote signer, so I think here you should also duplicate the logic (for redundancy and future buggy refactor proofing) - check

if !testMode && config.PrivateBls != ""

or something. Haven't thought this through, but I personally don't like any implicit stuff (like implicitly turning bls remote signer when some other flag is present. Prefer everything to have an "enabled" switch. Perhaps you can have a SignerType flag, and then crash when that thing is set to private-key and testMode is set to disabled?

test mode: bls private key
prod mode: bls sk file OR bls remote signer
?

Yes - I am not aware where test mode is used? maybe I think in inabox testing but yes that's the intention - in prod you can use either encrypted sk file or remote signer

so what I can do is - have an explicit flag which switches between sk file and bls remote signer. PrivateBls is not really used in prod. maybe @ian-shim can confirm but I think thats true.

maybe we should eventually remove out test node and use either sk file or remote signer as that clearly mimics prod usage

the ugly thing about testMode and prod mode with sk file is - both these cases populate privateBls and that's why checking testMode is not going to work here. since privateBls is populated in non testMode too. https://github.com/Layr-Labs/eigenda/pull/745/files#diff-923e406f4251573464f7748834b59f4747b3f6ec4eadda667614e2875a3b811aR166-R174

I think ideal would be to remove this testMode and eventually put the prod signing behind eigensdk as I said in above comment.

Test mode I think is just used to allow passing a sk directly? Think it should be allowed to simplify tests.

if that would remain - in current way - privateBls will be populated for both. I guess we could change eventually when we abstract out signing behind eigensdk

samlaf · 2024-11-12T17:46:37Z

node/node.go

+			},
+		)
+		if err != nil {
+			return nil, fmt.Errorf("failed to sign data: %w", err)


security check: are you sure that the error returned from your bls remote signer never leaks any sensitive info?

nope it does not. nothing ever returns any sensitive information from the signer itself.

node/node.go

ian-shim · 2024-11-12T18:17:39Z

node/node.go

@@ -114,6 +116,47 @@ func NewNode(reg *prometheus.Registry, config *Config, pubIPProvider pubip.Provi
 	// Create ChainState Client
 	cst := eth.NewChainState(tx, client)

+	var keyPair *core.KeyPair
+	var blsClient blssignerV1.SignerClient


I feel like this one-of pattern where one is expected to be nil and the other is not is very brittle.
There is this pattern in Config where PrivateBls can be empty and here where one of either keyPair or blsClient is nil.
Downstream, these attributes can't be assumed to be non-nil and there needs to be if/else nil check whenever they're accessed (or worse, forgetting to check this would result in panic).
Is there a blsClient abstraction that allows local signing with private bls key?

Agreed! This is what I was trying to describe in #745 (comment)

Is there a blsClient abstraction that allows local signing with private bls key?

I am thinking of having this abstraction in eigensdk and then you just initialize signer and based on config it would sign. Once I do that - we can switch it out.

Is this something that will be done & reflected in this PR? Or in a future PR?

pschork · 2024-11-15T06:58:36Z

Bump go-ethereum to latet to resolve unit tests

go-verkle was causing unit test failures with type *banderwagon.Element has no field or method BytesUncompressed

    go mod graph | grep go-verkle
    github.com/Layr-Labs/eigenda github.com/gballet/[email protected]
    github.com/ethereum/[email protected] github.com/gballet/[email protected]
    github.com/gballet/[email protected] github.com/crate-crypto/[email protected]
    github.com/gballet/[email protected] golang.org/x/[email protected]
    github.com/gballet/[email protected] github.com/bits-and-blooms/[email protected]
    github.com/gballet/[email protected] github.com/consensys/[email protected]
    github.com/gballet/[email protected] github.com/consensys/[email protected]
    github.com/gballet/[email protected] github.com/mmcloughlin/[email protected]
    github.com/gballet/[email protected] golang.org/x/[email protected]/gballet/[email protected] rsc.io/[email protected]

Resolve grpc.Dial deprecations due to cerbereus bumping grpc from v1.59.0 -> v1.64.1

Updates grpc.Dial -> grpc.NewClient
Removes withBlock() option from grpcConnection.Dial used by syntetic checks.

see https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md for why withBlock() was deprecated.

update docker file for private repo remove the go private deps api fix deps fix flags fix convert support latest signer support latest signer support latest signer replace with cerberus add cert file path operator id move log up move function up move function up fix incorrect logic clean and and make backward compatible clean up

go-verkle was causing unit test failures with `type *banderwagon.Element has no field or method BytesUncompressed` ``` go mod graph | grep go-verkle github.com/Layr-Labs/eigenda github.com/gballet/[email protected] github.com/ethereum/[email protected] github.com/gballet/[email protected] github.com/gballet/[email protected] github.com/crate-crypto/[email protected] github.com/gballet/[email protected] golang.org/x/[email protected] github.com/gballet/[email protected] github.com/bits-and-blooms/[email protected] github.com/gballet/[email protected] github.com/consensys/[email protected] github.com/gballet/[email protected] github.com/consensys/[email protected] github.com/gballet/[email protected] github.com/mmcloughlin/[email protected] github.com/gballet/[email protected] golang.org/x/[email protected] github.com/gballet/[email protected] rsc.io/[email protected] ``` Resolve grpc.Dial deprecations due to cerbereus bumping grpc from v1.59.0 -> v1.64.1 - Updates `grpc.Dial` -> `grpc.NewClient` - Removes `withBlock()` option from `grpcConnection.Dial` used by syntetic checks. see https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md for why `withBlock()` was deprecated.

ian-shim

one last comment

ian-shim · 2024-11-16T00:39:23Z

node/config.go

-			return nil, fmt.Errorf("could not read or decrypt the BLS private key: %v", err)
+		// If remote signer fields are empty then try to read the BLS key from the file
+		if !blsRemoteSignerEnabled {
+			kp, err := bls.ReadPrivateKeyFromFile(ctx.GlobalString(flags.BlsKeyFileFlag.Name), ctx.GlobalString(flags.BlsKeyPasswordFlag.Name))


BlsKeyFileFlag is still a required flag. Does it need to be an optional field now?

shrimalmadhur force-pushed the madhur/poc-remote-bls-signer branch 3 times, most recently from 283dc0e to 0b23714 Compare October 22, 2024 23:39

shrimalmadhur requested a review from ian-shim October 23, 2024 16:10

shrimalmadhur marked this pull request as ready for review October 23, 2024 20:45

shrimalmadhur changed the title ~~feat: poc bls remote signer~~ feat: bls remote signer Oct 23, 2024

v9n mentioned this pull request Nov 6, 2024

Support remote signer AvaProtocol/EigenLayer-AVS#31

Merged

samlaf reviewed Nov 12, 2024

View reviewed changes

ian-shim reviewed Nov 12, 2024

View reviewed changes

shrimalmadhur force-pushed the madhur/poc-remote-bls-signer branch from 0b23714 to 4077779 Compare November 13, 2024 07:03

shrimalmadhur requested review from ian-shim and samlaf November 13, 2024 07:20

pschork force-pushed the madhur/poc-remote-bls-signer branch from 4077779 to abe83f8 Compare November 15, 2024 06:56

shrimalmadhur and others added 3 commits November 15, 2024 13:50

addressed comments

952fe64

pschork force-pushed the madhur/poc-remote-bls-signer branch from abe83f8 to caa6681 Compare November 15, 2024 21:51

Update grpc.Dial to grpc.NewClient

59ed6ed

ian-shim reviewed Nov 16, 2024

View reviewed changes

Make bls keyfile/password flags option when using remote BLS signer

e0c80e7

ian-shim approved these changes Nov 16, 2024

View reviewed changes

pschork merged commit c70f576 into master Nov 16, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: bls remote signer #745

feat: bls remote signer #745

shrimalmadhur commented Sep 4, 2024

samlaf left a comment

samlaf Nov 12, 2024

samlaf Nov 12, 2024

samlaf Nov 12, 2024

samlaf Nov 12, 2024

shrimalmadhur Nov 13, 2024

samlaf Nov 12, 2024

shrimalmadhur Nov 13, 2024

samlaf Nov 12, 2024

shrimalmadhur Nov 13, 2024 •

edited

Loading

shrimalmadhur Nov 13, 2024

shrimalmadhur Nov 13, 2024

shrimalmadhur Nov 13, 2024 •

edited

Loading

samlaf Nov 13, 2024

shrimalmadhur Nov 13, 2024

samlaf Nov 12, 2024

shrimalmadhur Nov 13, 2024

ian-shim Nov 12, 2024 •

edited

Loading

samlaf Nov 12, 2024

shrimalmadhur Nov 13, 2024 •

edited

Loading

ian-shim Nov 15, 2024

pschork commented Nov 15, 2024

ian-shim left a comment

ian-shim Nov 16, 2024

feat: bls remote signer #745

feat: bls remote signer #745

Conversation

shrimalmadhur commented Sep 4, 2024

Why are these changes needed?

Checks

samlaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrimalmadhur Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrimalmadhur Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-shim Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrimalmadhur Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pschork commented Nov 15, 2024

ian-shim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrimalmadhur Nov 13, 2024 •

edited

Loading

shrimalmadhur Nov 13, 2024 •

edited

Loading

ian-shim Nov 12, 2024 •

edited

Loading

shrimalmadhur Nov 13, 2024 •

edited

Loading